Skip to content

rsa key validation test and additions to rsa fromdata test#339

Open
sebastian-carpenter wants to merge 1 commit into
wolfSSL:masterfrom
sebastian-carpenter:rsa-validate-tests
Open

rsa key validation test and additions to rsa fromdata test#339
sebastian-carpenter wants to merge 1 commit into
wolfSSL:masterfrom
sebastian-carpenter:rsa-validate-tests

Conversation

@sebastian-carpenter

@sebastian-carpenter sebastian-carpenter commented Dec 17, 2025

Copy link
Copy Markdown
Contributor

Adding tests for rsa key validation as well as adding some more cases to the rsa fromdata test.

Bugs:

  • WP segfaults when NULL is provided for param->data field (fixed)
  • General failure of rsa key validation (mostly fixed)
    • public key should have a few other tests
  • WP accepts any type of parameter value for rsa fields (fixed)

Additions (these are bugs too though):

  • Prime check in wp_rsa_validate against 133 primes. Later uses mp_prime_is_prime to check if n is composite.
  • The 'selection' variable used for rsa key importing was mostly unused, added section to make use of it.
    • This is to follow OSSL's implementation closer. Involved checking for D param, not just N and E.
  • No param may have more bits than the modulus.
  • Added checks that all CRT parameters are provided.
    • These checks vary between OSSL versions

Problems?

  • Should negative values be stored as 0? OSSL stores them as a negative and fails later but WP must store these values as unsigned. What a hassle! Updated to fail when negative value is received even though OpenSSL suceeds at first.

Comment thread src/wp_rsa_kmgmt.c
ok = 0;
}

if (ok) {
for(prime = 0; prime < VALIDATE_PRIMES_SIZE; prime++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not use mp_prime_is_prime() with 0 trials?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would fail before it tests primes because you can't have 0 trials (as far as I know).
OpenSSL does 5 rounds of miller-rabin anyway so I'd like to use mp_prime_is_prime() but I get different results between OpenSSL and wolfProvider.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up doing mp_prime_is_prime with 5 trials. Looks like I still need the table of primes to check the GCD though otherwise the results diverge from OSSL.

@sebastian-carpenter sebastian-carpenter force-pushed the rsa-validate-tests branch 5 times, most recently from 6dd7067 to 982f4fd Compare January 12, 2026 21:27
@sebastian-carpenter sebastian-carpenter marked this pull request as ready for review January 12, 2026 23:06
ColtonWilley
ColtonWilley previously approved these changes Feb 2, 2026
@padelsbach

Copy link
Copy Markdown
Contributor

@sebastian-carpenter, this looks important to integrate. Can you resolve the conflicts?

@sebastian-carpenter

Copy link
Copy Markdown
Contributor Author

@sebastian-carpenter, this looks important to integrate. Can you resolve the conflicts?

I'm working on some changes. I've got it rebased and just need to work through some issues I found.

updated rsa_key_import / validate to survive new tests
Copilot AI review requested due to automatic review settings July 1, 2026 22:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens wolfProvider’s RSA key validation and import behavior to better align with OpenSSL, and adds/extends unit tests to cover key integrity and EVP_PKEY_fromdata edge cases (NULL data, CRT completeness, bit-size bounds, negative values).

Changes:

  • Adds a new RSA key integrity unit test and registers it in the unit test harness.
  • Expands RSA fromdata tests with additional selections and parameter permutations (including NULL OSSL_PARAM data cases on newer OpenSSL).
  • Updates RSA key validation/import logic: additional modulus/exponent sanity checks, small-prime factor screening, CRT completeness checks, and stricter parameter type/value handling.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
test/unit.h Declares the new test_rsa_key_integrity unit test.
test/unit.c Registers the new RSA key integrity test in the test case table.
test/test_rsa.c Expands RSA fromdata coverage and adds a new RSA key integrity test helper suite.
src/wp_rsa_kmgmt.c Enhances RSA validation and hardens RSA parameter import/CRT verification.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/test_rsa.c
Comment on lines +2705 to +2730
static int test_rsa_key_integrity_single_factor(OSSL_PARAM_BLD* bld, BIGNUM* p,
BIGNUM* n, BIGNUM* e, BIGNUM* d)
{
int err = 0;
int ossl_err;
int wolf_err;
OSSL_PARAM* params = NULL;
EVP_PKEY_CTX* ctx_ossl = NULL;
EVP_PKEY_CTX* ctx_wolf = NULL;
EVP_PKEY* pkey_ossl = NULL;
EVP_PKEY* pkey_wolf = NULL;

err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_N, n) != 1;
err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_E, e) != 1;
err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_D, d) != 1;
err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_FACTOR1, p) != 1;

if (err == 0) {
params = OSSL_PARAM_BLD_to_param(bld);
err = params == NULL;
}
if (err == 0) {
ctx_ossl = EVP_PKEY_CTX_new_from_name(osslLibCtx, "RSA", NULL);
ctx_wolf = EVP_PKEY_CTX_new_from_name(wpLibCtx, "RSA", NULL);
err = (ctx_ossl == NULL) || (ctx_wolf == NULL);
}
Comment thread src/wp_rsa_kmgmt.c
Comment on lines +1194 to +1214
#define VALIDATE_PRIMES_SIZE 133
static const mp_digit validate_primes[VALIDATE_PRIMES_SIZE] = {
0x0002, 0x0003, 0x0005, 0x0007, 0x000B, 0x000D, 0x0011, 0x0013,
0x0017, 0x001D, 0x001F, 0x0025, 0x0029, 0x002B, 0x002F, 0x0035,
0x003B, 0x003D, 0x0043, 0x0047, 0x0049, 0x004F, 0x0053, 0x0059,
0x0061, 0x0065, 0x0067, 0x006B, 0x006D, 0x0071, 0x007F, 0x0083,
0x0089, 0x008B, 0x0095, 0x0097, 0x009D, 0x00A3, 0x00A7, 0x00AD,
0x00B3, 0x00B5, 0x00BF, 0x00C1, 0x00C5, 0x00C7, 0x00D3, 0x00DF,
0x00E3, 0x00E5, 0x00E9, 0x00EF, 0x00F1, 0x00FB, 0x0101, 0x0107,
0x010D, 0x010F, 0x0115, 0x0119, 0x011B, 0x0125, 0x0133, 0x0137,
0x0139, 0x013D, 0x014B, 0x0151, 0x015B, 0x015D, 0x0161, 0x0167,
0x016F, 0x0175, 0x017B, 0x017F, 0x0185, 0x018D, 0x0191, 0x0199,
0x01A3, 0x01A5, 0x01AF, 0x01B1, 0x01B7, 0x01BB, 0x01C1, 0x01C9,
0x01CD, 0x01CF, 0x01D3, 0x01DF, 0x01E7, 0x01EB, 0x01F3, 0x01F7,
0x01FD, 0x0209, 0x020B, 0x021D, 0x0223, 0x022D, 0x0233, 0x0239,
0x023B, 0x0241, 0x024B, 0x0251, 0x0257, 0x0259, 0x025F, 0x0265,
0x0269, 0x026B, 0x0277, 0x0281, 0x0283, 0x0287, 0x028D, 0x0293,
0x0295, 0x02A1, 0x02A5, 0x02AB, 0x02B3, 0x02BD, 0x02C5, 0x02CF,
0x02D7, 0x02DD, 0x02E3, 0x02E7, 0x02EF,
};

Comment thread src/wp_rsa_kmgmt.c
Comment on lines +1314 to +1321
/**
* Copy an unsigned value from an OSSL param into a provided RSA key parameter.
*
* @param [out] mp RSA key parameter.
* @param [in] param Parameter to copy value from.
* @return Size of mp in bits on success.
* @return -1 on failure.
*/
Comment thread test/test_rsa.c
Comment on lines +2585 to +2609
static int test_rsa_key_integrity_helper(OSSL_PARAM_BLD* bld, BIGNUM* p,
BIGNUM* q, BIGNUM* n, BIGNUM* e, BIGNUM* d, int expect_wolf_reject)
{
int err = 0;
int ossl_err;
int wolf_err;
OSSL_PARAM* params = NULL;
EVP_PKEY_CTX* ctx_ossl = NULL;
EVP_PKEY_CTX* ctx_wolf = NULL;
EVP_PKEY* pkey_ossl = NULL;
EVP_PKEY* pkey_wolf = NULL;

if (err == 0) {
err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_FACTOR1, p) != 1;
err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_FACTOR2, q) != 1;
err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_N, n) != 1;
err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_E, e) != 1;
err |= OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_RSA_D, d) != 1;
}
if (err == 0) {
params = OSSL_PARAM_BLD_to_param(bld);
err = params == NULL;
}

if (err == 0) {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants